Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Update Search component to support i18n #1192

Merged
merged 7 commits into from
May 5, 2021

Conversation

SirenaBorracha
Copy link
Contributor

@SirenaBorracha SirenaBorracha commented Apr 27, 2021

Summary

This is a quick update to the existing Search component to add support for i18n. I followed the pattern established in the DatePicker component

Related Issues or PRs

closes #1103

How To Test

Check out this branch and run yarn test to execute tests and yarn storybook to see the component in action.

Alternatively, you can scroll to the bottom of this page and click "Show environments" on the right above the comment input box to access this component in Storybook.

Screenshots

Screen Shot 2021-04-26 at 5 52 29 PM

…as Button text, update with test checking for ability to pass label, update stories with Spanish examples to match USWDS
@trussworks-infra-zz trussworks-infra-zz temporarily deployed to storybook April 27, 2021 19:13 Inactive
@trussworks-infra-zz trussworks-infra-zz temporarily deployed to storybook April 28, 2021 17:39 Inactive
@@ -73,7 +73,7 @@ export const Search = ({
/>
<Button type="submit">
<span className={isSmall ? 'usa-sr-only' : 'usa-search__submit-text'}>
Search
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So while this works and may be perfectly acceptable, I can see a use case where someone may want to the Button text to be different from the label.

There's also the thought of following convention for localizations which we established in #990. There we added an i18n object prop which contained all the localization values. Since we only have one value to worry about here, that might feel like a bit much.

My overall preference with this is to follow convention, even if it feels like overkill, in the name of uniform codebase. Plus, to handle the scenario of wanting label and button text to be different we would have to introduce a new prop anyways, so that would be half way to implementing our established i18n pattern.

@trussworks-infra-zz trussworks-infra-zz temporarily deployed to storybook April 29, 2021 19:58 Inactive
@trussworks-infra-zz trussworks-infra-zz temporarily deployed to storybook April 30, 2021 20:50 Inactive
@trussworks-infra-zz trussworks-infra-zz temporarily deployed to storybook May 3, 2021 21:12 Inactive
Copy link
Contributor

@brandonlenz brandonlenz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicely done! 🎉

@@ -44,6 +50,8 @@ export const Search = ({
deprecationWarning('Search property small is deprecated. Use size')
}

const buttonText = i18n?.buttonText || 'Search'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want, you can even declare a default for i18n (such as the EN_US used in DatePicker). Since this is only one field within i18n, I'm good with it as is.

@SirenaBorracha SirenaBorracha merged commit 5241d15 into main May 5, 2021
@SirenaBorracha SirenaBorracha deleted the ak-add-i18n-support-tosearch-component-1103 branch May 5, 2021 21:12
SirenaBorracha added a commit that referenced this pull request May 5, 2021
## [1.17.0](1.16.0...1.17.0) (2021-05-05)


### Features

* Checkbox Tile Variant ([#1104](#1104)) ([9936c4a](9936c4a))
* Implement ProcessListHeading subcomponent ([#1162](#1162)) ([964e34c](964e34c))
* New Component ProcessList MVP ([#1107](#1107)) ([1bc0f93](1bc0f93))
* New Component SiteAlert ([#1099](#1099)) ([c1e88e0](c1e88e0))
* New Component SummaryBox ([#1098](#1098)) ([b2279b4](b2279b4))
* Radio Button Tile Variant ([#1101](#1101)) ([a2f40a0](a2f40a0))
* Update Grid components to render any type of element ([#1166](#1166)) ([07468c8](07468c8)), closes [#1194](#1194)
* Update Search component to support i18n ([#1192](#1192)) ([5241d15](5241d15))
* Update Table to 2.10.0 implementation  ([#1110](#1110)) ([117a6c7](117a6c7))
brandonlenz pushed a commit that referenced this pull request May 12, 2021
* Update Search component to pass in label instead of hardcoded string as Button text, update with test checking for ability to pass label, update stories with Spanish examples to match USWDS

* Simplify localization interface, update test, update Storybook examples
brandonlenz pushed a commit that referenced this pull request May 12, 2021
## [1.17.0](1.16.0...1.17.0) (2021-05-05)


### Features

* Checkbox Tile Variant ([#1104](#1104)) ([9936c4a](9936c4a))
* Implement ProcessListHeading subcomponent ([#1162](#1162)) ([964e34c](964e34c))
* New Component ProcessList MVP ([#1107](#1107)) ([1bc0f93](1bc0f93))
* New Component SiteAlert ([#1099](#1099)) ([c1e88e0](c1e88e0))
* New Component SummaryBox ([#1098](#1098)) ([b2279b4](b2279b4))
* Radio Button Tile Variant ([#1101](#1101)) ([a2f40a0](a2f40a0))
* Update Grid components to render any type of element ([#1166](#1166)) ([07468c8](07468c8)), closes [#1194](#1194)
* Update Search component to support i18n ([#1192](#1192)) ([5241d15](5241d15))
* Update Table to 2.10.0 implementation  ([#1110](#1110)) ([117a6c7](117a6c7))
@haworku haworku added the i18n Relates to internationalization or localization standards label Jul 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
i18n Relates to internationalization or localization standards
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[feat] Add i18n support to Search Component
4 participants